Skip to content

Make can::Id usable as key in BTreeMap and HashMap #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 13, 2022

Conversation

usbalbin
Copy link
Contributor

Implement Ord and Hash for can::Id to make it usable as key in BTreeMap and HashMap

@usbalbin usbalbin requested a review from a team as a code owner May 10, 2022 06:55
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

@timokroeger
Copy link
Contributor

I think having Ord for StandardId and ExtendedId makes perfect sense.
For can::Id the question is if Ord should be implement according to CAN bus arbitration rules. In that case the automatic derive would not work.

@usbalbin
Copy link
Contributor Author

Does that mean that
Id::Standard(StandardId::ZERO) > Id::Standard(StandardId::MAX) should be true? Thus small number meaning important means "large"? :)

@usbalbin
Copy link
Contributor Author

usbalbin commented May 10, 2022

In that case, would it not make more sense if the ordering was the same for all three types?

@timokroeger
Copy link
Contributor

timokroeger commented May 10, 2022

Unfortunately arbitration rules are a bit more complex than that if I understand the CAN specification correctly.
When two messages are transmitted at the same time the ID with the first dominant bit (0bit) wins.
To compare standard and extended ids you would need to shift the standard ID by 18bits to align them with the MSB of the extended ID and also account for the IDE bit.

Example:
Std(0) < Ext(0) < Ext(1337) < Std(1) < Ext(555555) < Std(4)

@usbalbin
Copy link
Contributor Author

Oh, ok thanks
I will have a closer look at the protocol :)

@usbalbin
Copy link
Contributor Author

So something more like

impl Ord for Id {
    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
        let split_id = |id: &Id| {
            let (standard_id_part, ide_bit, extended_id_part) = match id {
                Id::Standard(StandardId(x)) => (*x, 0, 0),
                Id::Extended(x) => (
                    x.standard_id().0,
                    1,
                    x.0 & ((1 << 18) - 1), // Bit ID-17 to ID-0
                ),
            };
            (standard_id_part, ide_bit, extended_id_part)
        };

        split_id(self).cmp(&split_id(other))
    }
}

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, thank you!
I have not looked in detail but a couple of things that would be necessary for the merge would be:

  • Documentation on how to use this comparison and how it behaves with regard to the CAN arbitration rules and so on.
  • An entry in the changelog about this.

@timokroeger
Copy link
Contributor

Looks good to me now. Can you please fix the two tests too:

failures:
    can::id::tests::cmp_extended
    can::id::tests::cmp_standard

@usbalbin
Copy link
Contributor Author

I hope I have put the documentation in the right place. Otherwise please let me know.

The failing test have been removed, they were forgotten left overs from my first attempt

@usbalbin
Copy link
Contributor Author

Anything more I can do?

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a couple of tiny nitpicks that I can merge myself.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @usbalbin and @timokroeger!
bors r+

@bors bors bot merged commit 748f6f0 into rust-embedded:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants